Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nastevens (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Alternative to #29

try

Build failed

• continuous-integration/travis-ci/push

The changes generally look Ok, I had some extra concerns at first related to semver but 0.3 is not published yet so that should be OK.

Probably the main thing to consider changing here would be to do the additional conditional compilation to allow for completely dropping the sysfs gpio dep if the feature is not selected.

I question the choice to expose struct Pin with the same name for different backends. This seems unecessary and confusing. Instead, I feel it would be simpler (in terms of implementation and use) to expose two different structs. Making these look the same is what the traits are for and I fear using feature flags as is done here is primarily a recipe for confusion. Open to thoughts on this, this is just my thinking getting to this a bit delayed.

I question the choice to expose struct Pin with the same name for different backends.

yeah this is a good point, would renaming to CdevPin and SysfsPin for the impls work for you, then the feature flag is just selecting which is re-exported as Pin (which we need for backwards compatibility)?

Probably the main thing to consider changing here would be to do the additional conditional compilation to allow for completely dropping the sysfs gpio dep if the feature is not selected.

seems reasonable to me, i guess this would look like adding a gpio_sysfs feature, then what was re-exported as Pin by default would depend on the combination of flags?

tangentially (and outside the scope of this PR), is there a point at which we want to switch people from one to the other? and, is there some way we could do this automagically at runtime instead of requiring people to compile specifically for the gpio mode? (not that support for older kernels doesn't always require a GLIBC dance anyway)

@posborne the CI issue is fixed in #31 if you have any time to quickly review that

yeah this is a good point, would renaming to CdevPin and SysfsPin for the impls work for you, then the feature flag is just selecting which is re-exported as Pin (which we need for backwards compatibility)?

I think regardless we will not be backwards compatible, right unless someone changes the features around. Since we having a semver move to 0.3 I think it probably makes sense to introduce the breakage and avoid trying to do the rename. The names you propose look reasonable to me. Doing things this way also would allow enabling both features if desired (this could make sense for some people as people may want to use cdev for some new stuff but might still rely on udev rules to setup symlinks which would be used with sysfs).

tangentially (and outside the scope of this PR), is there a point at which we want to switch people from one to the other?

I think we just put the deprecation front and center and focus development on the newer stuff. The fact of the matter is that there are a lot of engineers who may want to use rust but will be continuing to work against systems that do not have cdev even after sysfs_gpio is deprecated in upstream kernels. Thinking specifically of 3.10 based systems.

These products should move to 4.9 LTSI but that's easier said than done depending on the SoC vendor and drivers in use. Right now I think we focus on making sure that people can use cdev and over push them that way; the hammer can come a bit later.

yeah this is a good point, would renaming to CdevPin and SysfsPin for the impls work for you, then the feature flag is just selecting which is re-exported as Pin (which we need for backwards compatibility)?

I think regardless we will not be backwards compatible, right unless someone changes the features around. Since we having a semver move to 0.3 I think it probably makes sense to introduce the breakage and avoid trying to do the rename. The names you propose look reasonable to me. Doing things this way also would allow enabling both features if desired (this could make sense for some people as people may want to use cdev for some new stuff but might still rely on udev rules to setup symlinks which would be used with sysfs).

IMO that make sense since we can break the backward comapibility. So I'll prepare commit that introduces SysfsPin and CdevPin enabled with different features. Does it work for you?

tangentially (and outside the scope of this PR), is there a point at which we want to switch people from one to the other?

I think we just put the deprecation front and center and focus development on the newer stuff. The fact of the matter is that there are a lot of engineers who may want to use rust but will be continuing to work against systems that do not have cdev even after sysfs_gpio is deprecated in upstream kernels. Thinking specifically of 3.10 based systems.

These products should move to 4.9 LTSI but that's easier said than done depending on the SoC vendor and drivers in use. Right now I think we focus on making sure that people can use cdev and over push them that way; the hammer can come a bit later.

IMO that make sense since we can break the backward comapibility. So I'll prepare commit that introduces SysfsPin and CdevPin enabled with different features. Does it work for you?

Sounds good to me.

Build succeeded

• continuous-integration/travis-ci/push